Skip to content

Misleading documentation for Websockets via .createServer#349

Merged
indexzero merged 2 commits into
http-party:masterfrom
itsjamie:patch-1
Mar 9, 2013
Merged

Misleading documentation for Websockets via .createServer#349
indexzero merged 2 commits into
http-party:masterfrom
itsjamie:patch-1

Conversation

@itsjamie
Copy link
Copy Markdown
Contributor

It's said its handled automatically, however from my experience this isn't the whole story, and to save someone else a headache this should be remedied in the docs.

Just some background I had a connect middleware for no-www, and options..

var options = {
    hostnameOnly: true,
    router: {
        'api.[snip]': '127.0.0.1:3001',
        '[snip]': '127.0.0.1:3000'
    }
};

var server = httpProxy.createServer(
    nowww(false),
    options
);

server.listen(80, function() {
  console.log('proxy up');
});

The websockets connections aren't made automatically because I was using middleware, so you need to add..

server.on('upgrade', function(req, socket, head) {
    server.proxy.proxyWebSocketRequest(req, socket, head);
});

This should just be documented somewhere is all. It would have saved me the hour of "hmm, why is this falling back to xhr-polling?" time and source-reading lines to see the "magic"

if (!callback) {
  proxy.proxyWebSocketRequest(req, socket, head);
}

But, more than willing to give up that hour for the total amount of time you've saved me 👍

@aeosynth
Copy link
Copy Markdown

👍 i'm not using any middleware and still had to manually listen for 'upgrade'

@indexzero
Copy link
Copy Markdown
Member

@jamie-stackhouse @aeosynth A documentation pull-request would be welcomed.

indexzero added a commit that referenced this pull request Mar 9, 2013
Misleading documentation for Websockets via .createServer
@indexzero indexzero merged commit c686ac7 into http-party:master Mar 9, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants